add quota - #462
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a quota management system, adding database tables, a QuotaModelMixin for tracking resource reservations, and corresponding API endpoints and tests. Key feedback includes correcting a typo in the migration base class name to avoid runtime errors, addressing a transaction atomicity issue in QuotaModelMixin.insert when no session is provided, and mitigating a potential SQL injection risk in reconcile_quota_reservations via table name whitelisting. Additionally, RSAKey and SSHKey should be added to DEFAULT_QUOTA_LIMITS for consistent tracking, and a misleading test name in test_quota.py should be renamed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
353ace7 to
7a25d74
Compare
1f2ec6d to
3518aa3
Compare
|
Sure! I'm generating a new review now. |
Reviewer's GuideIntroduce a generic quota subsystem with a QuotaLimit model, model mixin, DB migration, and user API endpoints, and apply quotas to compute nodes, node sets, load balancers, and secrets, along with tests and a minor SSH key insert conflict handling fix. Sequence diagram for quota-checked resource insertionsequenceDiagram
actor Client
participant Node as QuotaProtectedResource
participant QuotaModelMixin
participant DB as SQLSession
Client->>Node: insert(session)
Node->>QuotaModelMixin: _quota_check(session)
QuotaModelMixin->>QuotaModelMixin: _quota_limits(session)
alt field_limits_present
QuotaModelMixin->>DB: execute(SELECT fields FROM __tablename__ WHERE project_id)
DB-->>QuotaModelMixin: rows
QuotaModelMixin->>QuotaModelMixin: [sum field values + new value]
alt [current > limit]
QuotaModelMixin-->>Node: raise QuotaExceededError
Node-->>Client: QuotaExceededError
else [within field limits]
Note over QuotaModelMixin,Node: continue to count limits
end
end
alt count_limits_present
QuotaModelMixin->>Node: objects.count(session, filters)
Node-->>QuotaModelMixin: current_count
QuotaModelMixin->>QuotaModelMixin: [current_count + 1]
alt [current > limit]
QuotaModelMixin-->>Node: raise QuotaExceededError
Node-->>Client: QuotaExceededError
else [within count limits]
QuotaModelMixin-->>Node: quota ok
Node->>DB: insert(session)
DB-->>Node: success
Node-->>Client: inserted
end
else no_limits
QuotaModelMixin-->>Node: quota ok
Node->>DB: insert(session)
DB-->>Node: success
Node-->>Client: inserted
end
Entity relationship diagram for quota_limits and resourceserDiagram
QuotaLimit {
uuid uuid
project_id uuid
resource_name varchar
field_name varchar
limit int
}
Node {
uuid uuid
project_id uuid
cores int
}
LB {
uuid uuid
project_id uuid
}
SSHKey {
uuid uuid
project_id uuid
}
Node ||--o{ QuotaLimit : project_resource
LB ||--o{ QuotaLimit : project_resource
SSHKey ||--o{ QuotaLimit : project_resource
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 security issue, 4 other issues, and left some high level feedback:
Security issues:
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
General comments:
- SummaryController calls QuotaLimit.get_project_quota_summary(project_id), but QuotaLimit does not define this method in the PR, so either implement it or remove the controller endpoint that depends on it.
- SummaryRoute is defined but never attached under QuotaRoute (only
limitsis exposed), while the docstring claims to handle/v1/quota/reservations/summary/; wire this route into QuotaRoute (e.g.,summary = routes.route(SummaryRoute)) or update/remove the summary controller accordingly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- SummaryController calls QuotaLimit.get_project_quota_summary(project_id), but QuotaLimit does not define this method in the PR, so either implement it or remove the controller endpoint that depends on it.
- SummaryRoute is defined but never attached under QuotaRoute (only `limits` is exposed), while the docstring claims to handle `/v1/quota/reservations/summary/`; wire this route into QuotaRoute (e.g., `summary = routes.route(SummaryRoute)`) or update/remove the summary controller accordingly.
## Individual Comments
### Comment 1
<location path="migrations/0069-add-quota-tables-f8778e.py" line_range="24" />
<code_context>
+LOG = logging.getLogger(__name__)
+
+
+class MigrationStep(migrations.AbstarctMigrationStep):
+ def __init__(self):
+ self._depends = ["0068-fix-resource-status-hash-check-437c89.py"]
</code_context>
<issue_to_address>
**issue (bug_risk):** Base migration class name looks misspelled and may prevent the migration from loading.
This class inherits from `migrations.AbstarctMigrationStep`, which appears to be a typo and will fail if the actual base class is `AbstractMigrationStep` (or similar). Please verify the correct class name in `migrations` and update the inheritance to avoid migration discovery/import errors.
</issue_to_address>
### Comment 2
<location path="exordos_core/tests/functional/service/test_quota.py" line_range="129-127" />
<code_context>
+
+ first_node.delete()
+
+ def test_blocks_nodes_when_ram_limit_is_exceeded(
+ self,
+ _quota_limits,
+ node_factory_with_model,
+ ):
+ _, first_node = node_factory_with_model(cores=1, ram=2048)
+ _, second_node = node_factory_with_model(cores=1, ram=3072)
+
+ first_node.insert()
+ with pytest.raises(QuotaExceededError) as exc_info:
+ second_node.insert()
+
+ assert exc_info.value.resource_name == "nodes.ram"
+ assert exc_info.value.limit == 4096
+ assert exc_info.value.current == 5120
+
+ first_node.delete()
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a per-project isolation test for field-based node quotas
There’s already a project-isolation test for entity-count limits on LB (`test_limit_isolated_per_project`), but none for field-based node limits. Please add a test that sets cores/ram limits, creates nodes in two projects, and confirms that exceeding the limit in one project doesn’t affect the other, to validate that these quotas are scoped per project.
Suggested implementation:
```python
assert exc_info.value.resource_name == "nodes.cores"
assert exc_info.value.limit == 4
assert exc_info.value.current == 5
first_node.delete()
def test_node_field_quotas_are_isolated_per_project(
self,
_quota_limits,
node_factory_with_model,
):
# Assume the quota limits fixture sets per-project limits:
# cores limit: 4, ram limit: 4096 for each project.
project_a_uuid = sys_uuid.uuid4()
project_b_uuid = sys_uuid.uuid4()
# Project A: exceed cores/ram limits and ensure quota is enforced
_, a_node1 = node_factory_with_model(
project_uuid=project_a_uuid,
cores=2,
ram=2048,
)
_, a_node2 = node_factory_with_model(
project_uuid=project_a_uuid,
cores=2,
ram=2048,
)
_, a_node3 = node_factory_with_model(
project_uuid=project_a_uuid,
cores=1,
ram=1024,
)
a_node1.insert()
a_node2.insert()
# Exceeding the limit in project A should raise, and the error
# values should only reflect usage in project A.
with pytest.raises(QuotaExceededError) as exc_info:
a_node3.insert()
assert exc_info.value.resource_name in {"nodes.cores", "nodes.ram"}
assert exc_info.value.limit in {4, 4096}
assert exc_info.value.current in {5, 5120}
# Project B: usage should be independent of project A.
# Staying within limits in project B must not raise.
_, b_node1 = node_factory_with_model(
project_uuid=project_b_uuid,
cores=2,
ram=2048,
)
_, b_node2 = node_factory_with_model(
project_uuid=project_b_uuid,
cores=2,
ram=2048,
)
b_node1.insert()
b_node2.insert()
# Clean up nodes
a_node1.delete()
a_node2.delete()
a_node3.delete()
b_node1.delete()
b_node2.delete()
import uuid as sys_uuid
import pytest
from exordos_core.common import constants as c
from exordos_core.quota.dm.models import QuotaExceededError
from exordos_core.quota.dm.models import QuotaLimit
from exordos_core.user_api.network.dm.models import LB
```
The new test assumes:
1. `node_factory_with_model` accepts a `project_uuid` keyword argument that scopes nodes to a project. If the actual fixture uses a different parameter name (e.g. `project`, `project_id`, etc.), update the calls accordingly.
2. The `_quota_limits` fixture is already configuring per-project limits for node cores/ram (e.g. 4 cores, 4096 MB RAM). If the limits differ or are not per-project by default, configure `_quota_limits` in this test (or in the fixture) to set per-project `QuotaLimit` entries for `nodes.cores` and `nodes.ram`.
3. If your quota implementation exposes more precise attributes on `QuotaExceededError` (like separate `cores_limit` and `ram_limit`), you may want to split the assertions into two explicit checks (one for cores and one for RAM) instead of using the `in {}` sets.
</issue_to_address>
### Comment 3
<location path="exordos_core/tests/functional/restapi/quota/test_quota_api.py" line_range="30-39" />
<code_context>
return factory
+@pytest.fixture
+def node_factory_with_model():
+ def factory(
</code_context>
<issue_to_address>
**nitpick (testing):** Fixture `quota_limit_for_project` is currently unused
This fixture isn’t used in any tests, which likely means either a missing test or dead code. If you intended to test project-specific listing/filtering or the new summary endpoint, please add tests that consume this fixture; otherwise, remove it for clarity.
</issue_to_address>
### Comment 4
<location path="exordos_core/tests/functional/restapi/quota/test_quota_api.py" line_range="51-60" />
<code_context>
+class TestQuotaLimitsUserApi:
</code_context>
<issue_to_address>
**suggestion (testing):** Add API tests for the quota reservations summary endpoint and project_id filter
Current tests only cover `/v1/quota/limits/` and don’t exercise the new `SummaryController` reservations summary endpoint or its `project_id` filter. Please add tests for `/v1/quota/reservations/summary/` with and without `project_id`, asserting both response structure and that the aggregated quotas are correctly filtered by project.
Suggested implementation:
```python
class TestQuotaLimitsUserApi:
@staticmethod
def _limit_cmp_shallow(
a: tp.Dict[str, tp.Any],
b: tp.Dict[str, tp.Any],
) -> bool:
return all(
a.get(key, "") == b[key]
for key in (
"uuid",
"project_id",
)
)
class TestQuotaReservationsSummaryUserApi:
"""
Tests for `/v1/quota/reservations/summary/` endpoint, including project_id filtering.
"""
@staticmethod
def _reservation_summary_cmp_shallow(
a: tp.Dict[str, tp.Any],
b: tp.Dict[str, tp.Any],
) -> bool:
"""
Compare summary entries without being sensitive to extra fields.
Required fields:
- project_id
- resource
- total_reserved
"""
return (
a.get("project_id") == b["project_id"]
and a.get("resource") == b["resource"]
and a.get("total_reserved") == b["total_reserved"]
)
def test_reservations_summary_without_project_filter(
self,
client_user, # HTTP client for an authenticated user (kept consistent with existing tests)
quota_reservation_factory, # factory/fixture to create reservations
project_factory, # factory/fixture to create projects
) -> None:
"""
Ensure `/v1/quota/reservations/summary/` returns aggregated reservations
for all projects accessible to the user.
"""
# Arrange: create two projects and reservations on each
project_a = project_factory()
project_b = project_factory()
# reservations for project A
quota_reservation_factory(
project_id=project_a.uuid,
resource="cpu",
value=3,
)
quota_reservation_factory(
project_id=project_a.uuid,
resource="cpu",
value=2,
)
quota_reservation_factory(
project_id=project_a.uuid,
resource="memory",
value=1024,
)
# reservations for project B
quota_reservation_factory(
project_id=project_b.uuid,
resource="cpu",
value=5,
)
# Act
resp = client_user.get("/v1/quota/reservations/summary/")
# Assert: basic response structure
assert resp.status_code == 200
data = resp.json()
assert isinstance(data, list)
assert data, "Expected at least one summary entry"
# Each summary entry should have the required keys
for entry in data:
assert "project_id" in entry
assert "resource" in entry
assert "total_reserved" in entry
# Check that aggregation is correct for project A and project B
expected = [
{
"project_id": str(project_a.uuid),
"resource": "cpu",
"total_reserved": 5,
},
{
"project_id": str(project_a.uuid),
"resource": "memory",
"total_reserved": 1024,
},
{
"project_id": str(project_b.uuid),
"resource": "cpu",
"total_reserved": 5,
},
]
# Map returned data to (project_id, resource) -> total_reserved for easy comparison
actual_map = {
(entry["project_id"], entry["resource"]): entry["total_reserved"]
for entry in data
}
for e in expected:
key = (e["project_id"], e["resource"])
assert key in actual_map
assert actual_map[key] == e["total_reserved"]
def test_reservations_summary_with_project_filter(
self,
client_user,
quota_reservation_factory,
project_factory,
) -> None:
"""
Ensure `/v1/quota/reservations/summary/` with `project_id` returns only
aggregated reservations for the specified project.
"""
# Arrange: create two projects and reservations on each
project_a = project_factory()
project_b = project_factory()
# reservations for project A
quota_reservation_factory(
project_id=project_a.uuid,
resource="cpu",
value=3,
)
quota_reservation_factory(
project_id=project_a.uuid,
resource="cpu",
value=2,
)
quota_reservation_factory(
project_id=project_a.uuid,
resource="memory",
value=2048,
)
# reservations for project B (should NOT appear in filtered summary)
quota_reservation_factory(
project_id=project_b.uuid,
resource="cpu",
value=5,
)
# Act: filter by project A
resp = client_user.get(
"/v1/quota/reservations/summary/",
params={"project_id": str(project_a.uuid)},
)
# Assert: response structure
assert resp.status_code == 200
data = resp.json()
assert isinstance(data, list)
assert data, "Expected at least one summary entry for filtered project"
for entry in data:
assert entry["project_id"] == str(project_a.uuid)
assert "resource" in entry
assert "total_reserved" in entry
# Aggregation for project A only
expected = [
{
"project_id": str(project_a.uuid),
"resource": "cpu",
"total_reserved": 5,
},
{
"project_id": str(project_a.uuid),
"resource": "memory",
"total_reserved": 2048,
},
]
actual_map = {
(entry["project_id"], entry["resource"]): entry["total_reserved"]
for entry in data
}
# No entries for project B
for entry in data:
assert entry["project_id"] != str(project_b.uuid)
for e in expected:
key = (e["project_id"], e["resource"])
assert key in actual_map
assert actual_map[key] == e["total_reserved"]
```
Because we only see part of `TestQuotaLimitsUserApi` and the fixture names are inferred, you may need to:
1. **Adjust fixture names**:
- Replace `client_user` with the actual client fixture used in the rest of this file (e.g. `client`, `user_client`, etc.).
- Replace `quota_reservation_factory` and `project_factory` with the real factories/fixtures you already use to create reservations/projects or quota objects. If reservations are created via another helper (e.g. `create_quota_reservation`), wire that in instead.
2. **Align endpoint path and params**:
- Confirm the path for the summary endpoint (e.g. `"/v1/quota/reservations/summary/"` vs `"/v1/quota/reservations/summary"`), and adjust the strings accordingly.
- If your test client uses a different way to pass query params (e.g. `query_string` or `params` arg name), update the `client_user.get(...)` calls to match.
3. **Adapt field names to actual API response**:
- If the summary response uses different keys (e.g. `total` instead of `total_reserved`, `project_uuid` instead of `project_id`), update the assertions and expected dicts accordingly.
- If the response is wrapped (e.g. `{ "results": [...] }`), change `data = resp.json()` and subsequent assertions to index into the appropriate field.
4. **Reuse existing comparison helpers/conventions**:
- If you already have a helper for quota comparison (similar to `_limit_cmp_shallow`), consider reusing it or placing `_reservation_summary_cmp_shallow` next to related helpers and using it instead of manual map comparisons, to keep test style consistent across the file.
</issue_to_address>
### Comment 5
<location path="exordos_core/quota/dm/models.py" line_range="121-124" />
<code_context>
result = session.execute(
f"SELECT {fields} FROM {self.__tablename__} WHERE project_id = %s",
(self.project_id,),
)
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hey @akremenetsky, I've posted a new review for you! |
43ebf4b to
26a7670
Compare
Summary by Sourcery
Introduce a quota management system for core resources and expose it via the user API.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: